Skip to content

feat(sbom): add kosli attest sbom - #1166

Closed
AlexKantor87 wants to merge 8 commits into
mainfrom
claude/1162-attest-sbom
Closed

feat(sbom): add kosli attest sbom#1166
AlexKantor87 wants to merge 8 commits into
mainfrom
claude/1162-attest-sbom

Conversation

@AlexKantor87

@AlexKantor87 AlexKantor87 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Slice 2 of #1162. Adds the kosli attest sbom command, using the reader merged in #1165.

Draft, and it cannot pass CI yet. See "Why CI is red" below. The reason is a sequencing one the ticket already sets out, not a problem with the change.

What it does

kosli attest sbom --name my-sbom --sbom-file bom.json --flow f --trail t

Reads the SBOM, records what it says about itself, and uploads the file as an attachment.

Why only one file

The file is uploaded as-is, so the checksum recorded against it is the checksum of the file the customer handed us. They can verify it by hand.

That only holds for a single file. The CLI tars and gzips two or more attachments before upload, which would compress the SBOM and break the checksum that was just recorded for it. So --sbom-file and --attachments cannot be used together.

An already-compressed file is also rejected, because the format and the summary have to be read out of it.

Why the read is bounded rather than the file measured

The reader loads the whole file in one go, so the size has to be limited somewhere. An earlier version stat'd the path and then opened it separately to read, which does not actually bound anything: a file a build is still writing grows between the two calls, and a symlink can be repointed at a bigger file.

It now opens the file once and reads through an io.LimitReader capped one byte past the ceiling, then rejects on the length actually read. One byte past, so a file of exactly the ceiling is accepted and anything larger is not.

The ceiling is below the API's 10MB request limit rather than equal to it, because the JSON payload is counted against that limit alongside the file. Raising the ceiling needs direct-to-S3 upload, which is kosli-dev/server#6536.

Why the format and checksum are recorded twice

They go inside attestation_data, where the server's schema can enforce them, and again as the sbom_format and sbom_sha256 annotations, which is where a reader sees them on the trail page.

Holding one value in two places is usually a mistake. Here both come from the same read of the same file in the same pass, so they cannot drift apart: if the CLI is wrong they are wrong together rather than disagreeing.

Passing either key through --annotate is an error rather than being silently overwritten.

Why the URL says "system"

The slug is the attestation family, not the type. The server tells system types apart by type_name in the body, which is sbom here.

Why CI is red

One test reports an SBOM to a real Kosli server. CI runs against the current staging server image, which does not know what an sbom attestation is until https://github.com/kosli-dev/server/pull/6863 merges and deploys, so the POST comes back "System attestation type 'sbom' does not exist".

That test is skipped for now with the condition written into the skip message. Un-skipping it is a precondition for merging, not optional cleanup — it is the only test that exercises the command against a real server. Everything else in the file uses --dry-run and passes today.

The ticket already requires this order: the command must not be released until the server side is in production, because a released command pointed at a server that does not know the type fails for every customer who tries it.

What was run

Every test in the file except the skipped round-trip, through the same harness CI uses, with the environment the Makefile sets (KOSLI_TESTS=true plus the fake GitHub CI variables). All pass.

That environment matters. Without those variables a [warning] Repo information will not be reported line lands ahead of every error message, and each exact-match golden fails on it. A bare go test therefore looks broken when nothing is.

The full local stack still cannot run here: it wants a linux/amd64 server image, and building the server on an arm64 Mac produces an arm64 one the compose stack refuses. The tests that need a live server are the round-trip test, which is skipped anyway.

Also run: go build ./..., go vet ./..., gofmt, golangci-lint on the changed packages, and kosli attest sbom --help.

Each new guard was mutation-tested — the size comparison flipped from > to >=, the +1 dropped from the read limit, the regular-file check removed, and the annotation call removed. Every one of those turns a test red, so none of the tests are decoration.

Reports a software bill of materials as its own attestation type, using
the reader added in #1165.

The file is uploaded as it is, so the checksum recorded against it is the
checksum of the file the customer supplied and they can verify it by
hand. That is why only one file is allowed: the CLI tars and gzips two or
more attachments, which would compress the SBOM and break that checksum.
--sbom-file and --attachments are mutually exclusive for the same reason.

Size is checked before the file is read. The reader loads the whole file
in one go, so checking afterwards would pull a large file into memory
before the friendlier error could be produced. The ceiling leaves room
under the API's 10MB request limit, because the JSON payload is counted
alongside the file.

The format and the file checksum are recorded twice on purpose: inside
attestation_data, where the server's schema can enforce them, and as
annotations, which is where a reader sees them on the trail page. Both
come from the same read of the same file, so they cannot disagree. A
caller passing either annotation key gets an error rather than having
their value silently replaced.

The URL slug is the attestation family rather than the type. The server
tells system types apart by type_name in the body.
@github-actions github-actions Bot added go Pull requests that update go code feat labels Sep 11, 2026
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AlexKantor87's task in 4m 12s —— View job


Review: feat(sbom): add kosli attest sbom

  • Static analysis (go build, go vet, go mod tidy)
  • Read the changed files
  • Review posted (4 inline comments + summary below)

Phase 1 — static analysis

Check Result
go build ./... clean
go vet ./... clean
go vet ./cmd/kosli ./internal/sbom (incl. test files) clean
go mod tidygit diff --exit-code go.mod go.sum clean, no drift

Not run: gofmt -l and make lint — both blocked by the tool allowlist in this environment (gofmt/golangci-lint aren't permitted Bash commands here). Worth confirming locally; go vet passing is a weaker signal than either.

Phase 2 — findings

Four inline comments, all minor. Nothing blocking, and no correctness bug found in the command path.

  1. attestSbom_test.go:104-109the "more than one attachment" case is byte-identical coverage to the case above it. MuXRequiredFlags counts changed flags, not values (cli_utils.go:308-318), so a second --attachments value changes nothing. The test name advertises a scenario nothing tests.
  2. attestSbom_test.go:143-150 — the at-limit fixture (9MB of NULs) falls to ProcessSBOM's tag-value default branch, which ReplaceAlls the whole buffer (a second 9MB allocation) plus two more full scans, every suite run. Seeding the file with { routes it to the JSON probe, fails on byte two, and lets the golden assert a specific message instead of failed to parse SBOM file — which currently matches whichever branch the zeros happen to hit.
  3. attestSbom.go:258fmt.Sprintf("%x", …) vs the repo's hex.EncodeToString (internal/digest/digest.go:414). Identical output; consistency only.
  4. attestSbom_test.go:180 — local sbom shadows the package name the sibling file imports. Legal today, a compile error the day this test needs the package.

Things I checked that hold up

  • The pinned digest is real. sha256sum cmd/kosli/testdata/sbom/cyclonedx.jsondb09ef115d88e48a5ef553b21a88ccdc15b3df700e0b7c3736e2ef1024d26d9c, matching both original_fingerprint and sbom_sha256 in the goldens. The two now come from one buffer and one asserted value.
  • The duplicated fixtures are gone. compressed.json.gz is 8 bytes (87bcdd44…) and no longer a copy of internal/sbom/testdata/gzipped.json.gz (6e7a19fe…). The other three are purpose-written minimal documents.
  • TestSbomIsUploadedUncompressed is safe outside the suite. logger is assigned in main.go's init(), so the standalone test can't nil-panic on getPathOfEvidenceFileToUpload's logger.Debug. The t.Cleanup is registered before the assertions that can FailNow, and os.Remove("") on the error path is harmless.
  • The spec.json regeneration is genuinely one entry. Diff stat is +96/−0 — the indent=2 + trailing-newline fix in bootstrap.py did eliminate the reindent churn, and the 25 results.tsv rows all show the empty-value rejection firing ahead of the MuX check (including --attachments, which is the one I'd expected to report the wrong error).

Worth acknowledging

The annotate fix in 765b13c is the good kind: processAnnotations returning its argument means the payload map and the --annotate map are the same map, so writing into it seeded the very map rejectReservedAnnotations checks — a second run refused a key nobody supplied. Building a fresh map removes the class, and the comment explains why it copies rather than just noting that it does.

Same for 5211c93: prepareAttestationForm(o.payload, []string{o.sbomFilePath}) makes "exactly one attachment" hold by construction, instead of by the conjunction of a validator and a field that happens to be empty. Given that the entire verifiability claim rests on getPathOfEvidenceFileToUpload taking the len == 1 branch, that's the right place to spend a line.

The io.LimitReader(file, max+1) read off a single os.Open handle, with file.Stat() rather than os.Stat(path), is also worth noting — it's a real bound rather than a claim about a size measured earlier, and the +1 is what makes the ceiling the largest accepted size.

On CI

TestAttestSbomRoundTrip is still skipped pending kosli-dev/server#6863, with the ticket in the skip message so it's greppable. That remains the merge precondition the PR description states — everything else in the file runs without the server type.
· branch claude/1162-attest-sbom

Comment thread cmd/kosli/attest.go
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom_test.go
Comment thread cmd/kosli/attestSbom.go Outdated
Addresses the automated review.

The flag audit had no entry for this command, so a third test was failing
for a reason unrelated to the server sequencing. Regenerated.

The interesting half of this slice is now tested. Three dry-run cases
assert the URL, the type name, the parsed format, the fingerprint, the
package count and both annotations. They need a server for the flow and
trail, but not one that knows the sbom type, so they stay green until the
server side ships. Only the round trip is blocked now.

The file was read twice, once to fingerprint and once to parse, so the
recorded fingerprint could describe different bytes from the recorded
summary. internal/sbom gains a bytes entry point and the command reads
once. A path that is not a regular file is refused: a directory or a pipe
reports size zero, walked past the size guard and was then read unbounded.

The reserved annotation check moves to PreRunE. It needs nothing from the
file, and a typo should not cost a repository walk and a pass over nine
megabytes first.

Test fixtures were byte-identical copies of ones in internal/sbom, and the
gzip case duplicated an assertion made there. Dropped, and the two that
remain are minimal documents. The oversize file is now made with truncate
rather than by building nine megabytes of string.

Every error golden here was wrong: errors from this command carry a
"[kosli attest sbom flow= trail=]" prefix that I had not accounted for.
All six were corrected against the command's actual output.
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread internal/sbom/sbom.go Outdated
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom_test.go Outdated
AlexKantor87 and others added 2 commits September 11, 2026 20:51
The six error goldens carried a "[kosli attest sbom flow=... trail=...]"
prefix that the test harness never produces. The prefix comes from
enrichError, which is only called from innerMain; the suite calls
root.ExecuteC() directly, so the output is the bare message. The prefix
was added last round after checking against the built binary, which does
go through innerMain -- the wrong execution path to verify against.

The size guard stat'd the path and then opened it again to read, so the
bound it promised did not hold for a file still being written or a
symlink repointed between the two calls. It is now one handle: stat it
for IsRegular, then read through an io.LimitReader capped one byte past
the limit and reject on the length actually read. The size rule lives in
one place and the error message names the limit rather than an observed
size that may already be stale.

Also:
- annotate no longer returns an error it cannot produce, now that the
  reserved-key rejection runs in PreRunE.
- processSBOM is renamed ProcessSBOM, removing a pass-through with one
  caller.
- attestSbomLongDesc names --attachments as unusable here, which the
  one-file rule implied but never said.
- A file of exactly the limit is now tested from the accepting side, so
  the comparison is pinned as > rather than >=.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite runs against the current staging server image, which has no
sbom system attestation type, so the POST comes back "System attestation
type 'sbom' does not exist" and the job is red for a reason no change in
this repo can fix. The skip states the condition instead.

This is the only test that exercises the command against a real server,
so un-skipping it is a precondition for merging, not optional cleanup.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom_test.go Outdated
The goldens matched the fingerprint against [a-f0-9]{64}, which is true
of the digest of anything: the tarred bundle, the payload, a copy with
the BOM stripped. The command's whole promise is that the recorded
checksum is the checksum of the file the customer supplied, and a test
on the shape of a hash cannot check that. Both goldens now pin the
fixture's real digest. Hashing different bytes turns them red; under the
old regex it did not.

Nothing tested the reason the command refuses a second attachment. One
attachment is uploaded as it is; two are tarred and gzipped while
sbom_sha256 still describes the uncompressed original, and nothing
downstream fails. The dry-run goldens cannot see it because a multipart
request logs only its JSON fields. There is now a direct test, which
also asserts that two attachments ARE packaged, so it cannot pass by
packaging having stopped altogether.

Also:
- Five error cases carried --dry-run that did nothing: each fails before
  the request is built. It was not only noise. --dry-run is the mode
  where those command lines exit 0 for a real user, so wantError held
  only because the harness skips innerMain.
- A directory now says so, rather than reporting the generic
  not-a-regular-file message a user then has to interpret. It is the
  case reached by accident, when tab-completion stops a path short.
- The skip on the round-trip test names the server change it waits on,
  so it is greppable and shows up in go test -v output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread cmd/kosli/testdata/empty-flag-audit-coverage.json
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom_test.go
Comment thread cmd/kosli/attestSbom_test.go Outdated
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread cmd/kosli/attestSbom.go
The gzip fixture and the test that used it were deleted while reworking
this file two rounds ago. internal/sbom still tests that a gzipped file
is refused, but nothing tested that the command reports it, which is the
message a user actually sees. Both are back, along with a case for more
than one attachment: the existing test passed a single --attachments, so
the repeated form was never exercised.

Nothing asserted that a caller's own --annotate keys survive alongside
the two derived from the file. Replacing the merge in annotate with a
fresh map assignment passed every test in the file while silently
dropping them. Two cases now cover it, kept separate because map key
order in the payload is not stable.

spec.json had no attest sbom entry while the coverage file did, so
audit.py refused to run at all. Rather than write the entry by hand,
bootstrap.py now knows the fixture for --sbom-file, so regenerating
produces it. The entry records baseline_ok true because it was generated
against a server carrying the sbom type, which is the state at merge.

Also:
- annotate says it must run after CommonAttestationOptions.run, which
  assigns Annotations wholesale. The nil guard made the order look
  optional.
- The size comment no longer implies the 10MB limit is accounted for.
  --user-data rides in the same body and can pass the ceiling on its own.
- The help text names gzip, which is what the parser detects. A zip is
  refused by the generic unrecognised-file message.
- TestSbomIsUploadedUncompressed moved out of the suite, so it no longer
  needs a server to assert local logic, and registers its cleanup before
  the assertions that can abort.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread hack/empty-flag-audit/bootstrap.py
Comment thread internal/sbom/sbom.go
…ainst

payload.Annotations and the --annotate flag map are the same map, because
processAnnotations returns its argument. Writing the two derived keys into
it put them where rejectReservedAnnotations reads, so running one command
object twice refused the second run with "annotation key 'sbom_format'
cannot be provided with --annotate" for a key nobody supplied. Building a
new map decouples them. Reproduced both ways before and after the change.

The short description ended with one trailing space where most siblings
have two. Long is Short plus the rest of the text, and two trailing
spaces are markdown's hard line break, so with one the first sentence
merged into the next paragraph wherever the help is rendered as markdown.

Regenerating spec.json rewrote all 4680 lines. Not reordering: every
existing entry is identical in content and key order. bootstrap.py writes
indent=1 and no trailing newline, while the committed file is indent=2
with one, so every regeneration reformatted the whole file and buried the
entry that changed. It now writes what is already committed, and the diff
against main is 96 added lines with none removed.

ProcessSBOMFile has no caller left outside this package's tests, and it
reads a whole file with no ceiling next to a command that caps its reads.
Its doc comment now says so. Kept rather than deleted: ten tests call it,
including one for a missing file, which is the read path ProcessSBOM
cannot cover.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread cmd/kosli/attestSbom.go Outdated
Comment thread cmd/kosli/attestSbom_test.go
Comment thread internal/sbom/sbom.go Outdated
The SBOM path was appended onto o.attachments, so "exactly one
attachment" held only because a flag check left that field empty. Running
the same options struct twice made it [sbom, sbom], which is tarred and
gzipped while sbom_sha256 still describes the uncompressed original. That
is the same re-entrancy as the annotation fix in the previous commit, but
it fails silently: the attestation is simply wrong rather than refused.

Passing a one-element slice removes the class. o.attachments is now
neither read nor written on this path, so nothing that fills it first can
raise the count.

The gzip fixture was an 8KB blob, byte-identical to
internal/sbom/testdata/gzipped.json.gz. The rejection matches on the
first two bytes before anything is decompressed, so the fixture is now
the eight magic bytes: same path exercised, readable in git show, and no
second copy of a file that could drift from the original.

ProcessSBOMFile's godoc opened twice with its own name after the last
change, reading as a merge artifact and putting the caveat before the
description. The paragraphs are swapped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread cmd/kosli/attestSbom_test.go
Comment thread cmd/kosli/attestSbom.go
Comment thread cmd/kosli/attestSbom_test.go
Comment thread cmd/kosli/attestSbom_test.go
@AlexKantor87

Copy link
Copy Markdown
Contributor Author

Closing in favour of #1168, which is the same code squashed to one commit.

Five review rounds and eight commits made this one hard to read as a whole. The replacement also stops skipping the end-to-end test: CI on it is red until the server side (kosli-dev/server#6863) is on staging, which is the honest state rather than a green tick that depends on the one real test not running.

All 32 review threads here were answered and resolved before closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant